Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

[Impeller] Include AndroidSurfaceVulkanImpeller behind a flag #42033

Merged
merged 1 commit into from
May 15, 2023

Conversation

CaseyHillers
Copy link
Contributor

@CaseyHillers CaseyHillers commented May 15, 2023

Fixes b/282290672

Google Testing currently does not support vulkan, and constructors like this need to be behind a flag.

Pre-launch Checklist

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • I read the [Tree Hygiene] wiki page, which explains my responsibilities.
  • I read and followed the [Flutter Style Guide] and the [C++, Objective-C, Java style guides].
  • I listed at least one issue that this PR fixes in the description above.
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See [testing the engine] for instructions on writing and running engine tests.
  • I updated/added relevant documentation (doc comments with ///).
  • I signed the [CLA].
  • All existing and new tests are passing.

* Google Testing currently doesn't support vulkan
@flutter-dashboard
Copy link

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

@CaseyHillers
Copy link
Contributor Author

\cc @iskakaushik as FYI. I duplicated your TODO from below on wiring up a flag for impeller here.

@CaseyHillers
Copy link
Contributor Author

I'm going to submit as is to let this roll overnight to unblock Google testing. If there's a need for tests, I'll follow up in a separate PR.

@Hixie
Copy link
Contributor

Hixie commented May 15, 2023

FWIW, looks like in #37486 we used #if IMPELLER_ENABLE_VULKAN rather than #if false.

test-exempt: removes code

That said, it would be good to add testing for both of these code paths.

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 15, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 15, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 15, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 15, 2023
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request May 15, 2023
…126819)

flutter/engine@d685520...7bd7482

2023-05-15 [email protected] Roll Dart SDK from 7d6324d5488b to c777d54743e1 (1 revision) (flutter/engine#42038)
2023-05-15 [email protected] [Impeller] Fix DrawPaint advanced blends on iOS. (flutter/engine#42032)
2023-05-15 49699333+dependabot[bot]@users.noreply.github.com Bump google/mirror-branch-action from 1.0 to 2.0 (flutter/engine#42037)
2023-05-15 [email protected] Roll Skia from 0c2083a60b66 to 4becb53e3c21 (2 revisions) (flutter/engine#42036)
2023-05-15 [email protected] Roll Fuchsia Linux SDK from jXrn0SP_MnSmhkz_P... to EweLgJoiYUDok2vyU... (flutter/engine#42035)
2023-05-15 [email protected] Roll Skia from 0410e5d9ec03 to 0c2083a60b66 (1 revision) (flutter/engine#42034)
2023-05-15 [email protected] [Impeller] Include AndroidSurfaceVulkanImpeller behind a flag (flutter/engine#42033)
2023-05-15 [email protected] Reland "Remove GN staging flag for save layer bounds" (flutter/engine#42029)

Also rolling transitive DEPS:
  fuchsia/sdk/core/linux-amd64 from jXrn0SP_MnSm to EweLgJoiYUDo

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
@dnfield
Copy link
Contributor

dnfield commented May 15, 2023

FWIW, looks like in #37486 we used #if IMPELLER_ENABLE_VULKAN rather than #if false.

test-exempt: removes code

That said, it would be good to add testing for both of these code paths.

The #if false is because Vulkan on Android doesn't quite work yet. @chinmaygarde is chasing down the last of a bug or two to get this working right. Once it's working at all we can change the false to IMPELLER_ENABLE_VULKAN - but until then we don't want end users turning vulkan on by accident.

@Hixie
Copy link
Contributor

Hixie commented May 15, 2023

why do we have IMPELLER_ENABLE_VULKAN in some places but not others?

@dnfield
Copy link
Contributor

dnfield commented May 17, 2023

why do we have IMPELLER_ENABLE_VULKAN in some places but not others?

Vulkan is used on platforms/test harnesses besides Android.

Right now, we don't want anyone to turn Vulkan on on Android unless they have rebuilt the engine.

CaseyHillers pushed a commit to CaseyHillers/flutter that referenced this pull request May 24, 2023
…lutter#126819)

flutter/engine@d685520...7bd7482

2023-05-15 [email protected] Roll Dart SDK from 7d6324d5488b to c777d54743e1 (1 revision) (flutter/engine#42038)
2023-05-15 [email protected] [Impeller] Fix DrawPaint advanced blends on iOS. (flutter/engine#42032)
2023-05-15 49699333+dependabot[bot]@users.noreply.github.com Bump google/mirror-branch-action from 1.0 to 2.0 (flutter/engine#42037)
2023-05-15 [email protected] Roll Skia from 0c2083a60b66 to 4becb53e3c21 (2 revisions) (flutter/engine#42036)
2023-05-15 [email protected] Roll Fuchsia Linux SDK from jXrn0SP_MnSmhkz_P... to EweLgJoiYUDok2vyU... (flutter/engine#42035)
2023-05-15 [email protected] Roll Skia from 0410e5d9ec03 to 0c2083a60b66 (1 revision) (flutter/engine#42034)
2023-05-15 [email protected] [Impeller] Include AndroidSurfaceVulkanImpeller behind a flag (flutter/engine#42033)
2023-05-15 [email protected] Reland "Remove GN staging flag for save layer bounds" (flutter/engine#42029)

Also rolling transitive DEPS:
  fuchsia/sdk/core/linux-amd64 from jXrn0SP_MnSm to EweLgJoiYUDo

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants